Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Fix slice op issues #13760 #14403

Merged
merged 1 commit into from
Mar 18, 2019
Merged

Fix slice op issues #13760 #14403

merged 1 commit into from
Mar 18, 2019

Conversation

mseth10
Copy link
Contributor

@mseth10 mseth10 commented Mar 12, 2019 •
edited
Loading

Description

This PR fixes nd.slice for erroneous output in the following case: begin=end ( fixes #13760 )
Added unit test for the same.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • end=-1 with negative step is a valid input
  • begin=end throws invalid input error

@mseth10 mseth10 mentioned this pull request Mar 12, 2019
Closed
6 tasks
@mseth10
Copy link
Contributor Author

mseth10 commented Mar 12, 2019

This PR addresses the comments posted on an earlier PR #14107 (closed).

@mseth10
Copy link
Contributor Author

mseth10 commented Mar 12, 2019

Please review @reminisce @szha @anirudh2290

@karan6181
Copy link
Contributor

@mxnet-label-bot add [Operator, pr-awaiting-review]

@marcoabreu marcoabreu added Operator pr-awaiting-review PR is waiting for code review labels Mar 13, 2019
wkcn
wkcn approved these changes Mar 14, 2019
View reviewed changes
Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix! LGTM

yuxihu
yuxihu approved these changes Mar 14, 2019
View reviewed changes
Copy link
Member

@yuxihu yuxihu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

apeforest
apeforest suggested changes Mar 14, 2019
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are so many if/else/elseif branches in this piece of code. I know it fixes the two cases you mentioned in the description. However, instead of patching, could you please find ways to simplify the logic to fix more general cases? If not, can you add comment to the end of each branch such as // end of if (s < -1) to make the code more readable.

@mseth10 mseth10 changed the title Fix slice op issues #13760 and #14105 Fix slice op issues #13760 Mar 15, 2019
@mseth10 mseth10 force-pushed the fix-issue-13760-14105 branch 4 times, most recently from e0acfda to 65df5d1 Compare March 15, 2019 18:58
@mseth10
Copy link
Contributor Author

mseth10 commented Mar 15, 2019

There are so many if/else/elseif branches in this piece of code. I know it fixes the two cases you mentioned in the description. However, instead of patching, could you please find ways to simplify the logic to fix more general cases? If not, can you add comment to the end of each branch such as // end of if (s < -1) to make the code more readable.

@apeforest Simplified the logic and refactored the code. Also added comments. Please review.

apeforest
apeforest reviewed Mar 15, 2019
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks elegant! Thanks a lot for going the extra mile to make the code more clean and easy to read. Only one comment about unit test, otherwise it's good to ship.

apeforest
apeforest approved these changes Mar 16, 2019
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mseth10
begin=end not a valid input
1625f06 
refactoring logic for indexing
@apeforest apeforest merged commit d671528 into apache:master Mar 18, 2019
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
@mseth10 @vdantu
begin=end not a valid input (apache#14403)
fca242f 
refactoring logic for indexing
ZhennanQin pushed a commit to ZhennanQin/incubator-mxnet that referenced this pull request Apr 3, 2019
@mseth10 @ZhennanQin
begin=end not a valid input (apache#14403)
da49c3f 
refactoring logic for indexing
nswamy pushed a commit that referenced this pull request Apr 5, 2019
@mseth10 @nswamy
begin=end not a valid input (#14403)
ffc0708 
refactoring logic for indexing
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@mseth10
begin=end not a valid input (apache#14403)
433bd8b 
refactoring logic for indexing
@mseth10 mseth10 deleted the fix-issue-13760-14105 branch June 1, 2020 10:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@apeforest apeforest apeforest approved these changes

@wkcn wkcn wkcn approved these changes

@yuxihu yuxihu yuxihu approved these changes

@anirudh2290 anirudh2290 anirudh2290 approved these changes

Assignees
No one assigned
Labels
Operator pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nd.slice does not return empty tensor when begin=end
7 participants
@mseth10 @karan6181 @anirudh2290 @yuxihu @wkcn @apeforest @marcoabreu